INT-4162: TCP/UDP DSL #1966

garyrussell · 2016-11-07T20:28:37Z

JIRA: https://jira.spring.io/browse/INT-4162

INT-4162: IP DSL - Phase I Connection Factories

Phase 2 - Adapters/Gateways

Phase 3 - UDP

artembilan · 2016-11-07T21:15:14Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/AbstractConnectionFactorySpec.java

+			<S extends AbstractConnectionFactorySpec<S, C>, C extends AbstractConnectionFactory>
+		extends IntegrationComponentSpec<S, C> {
+
+	protected C target;


There is already target for the same purpose in the super IntegrationComponentSpec

artembilan · 2016-11-07T21:16:21Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/AbstractConnectionFactorySpec.java

+
+	protected C target;
+
+	public AbstractConnectionFactorySpec(C connectionFactory) {


Typically I do all the Spec only visible for the factory methods to instantiate.
Is there any purpose to let this be created directly from end-user level?

artembilan · 2016-11-07T21:20:53Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/AbstractConnectionFactorySpec.java

+	@Override
+	protected C doGet() {
+		if (getId() != null) {
+			this.target.setBeanName(getId());


getId() is fully orthogonal to the setId() . Since our target is populated from the ctor, it looks like we can directly call this setBeanName() from the id() and get rid of this doGet() altogether.
WDYT?

artembilan · 2016-11-07T21:23:47Z

...-ip/src/main/java/org/springframework/integration/ip/dsl/TcpClientConnectionFactorySpec.java

+		this(host, port, false);
+	}
+
+	public TcpClientConnectionFactorySpec(String host, int port, boolean nio) {


Should this be public?
Then why the previous one isn't?

artembilan · 2016-11-07T21:26:33Z

spring-integration-ip/src/main/java/org/springframework/integration/ip/dsl/Tcp.java

+	 * Create a server spec that uses NIO.
+	 * @param port the port to listen on.
+	 * @param <S> the spec type.
+	 * @param <C> the connection factrory type.


Typo: factrory

artembilan · 2016-11-07T21:50:58Z

spring-integration-ip/src/main/java/org/springframework/integration/ip/dsl/package-info.java

+/**
+ * Provides TCP/UDP Component support for the Java DSL.
+ */
+package org.springframework.integration.ip.dsl;


New line in the end of each class.

artembilan · 2016-11-07T21:53:02Z

...ration-ip/src/main/java/org/springframework/integration/ip/tcp/TcpSendingMessageHandler.java

 				}
 				if (this.serverConnectionFactory != null) {
-					this.serverConnectionFactory.stop(callback);
+					this.serverConnectionFactory.stop();


After changes to this method, the callback is out of use. I think we should call it in the end of the method anyway.
From it even possible to avoid code duplication since stop() without callback does the same now.

artembilan · 2016-11-07T21:56:17Z

...-integration-ip/src/test/java/org/springframework/integration/ip/dsl/IpIntegrationTests.java

+		@Bean
+		public IntegrationFlow inTcpGateway() {
+			return IntegrationFlows.from(Tcp.inboundGateway(server1()))
+					.transform(new ObjectToStringTransformer())


Nit pick. We have Transformers factory.

artembilan · 2016-11-07T21:57:15Z

...-integration-ip/src/test/java/org/springframework/integration/ip/dsl/IpIntegrationTests.java

+
+		@Bean
+		public IntegrationFlow outUdpAdapter() {
+			return f -> f.handle(Udp.outboundAdapter("headers['udp_dest']"));


Can we consider to have Expression and Function factory method variants ?

artembilan · 2016-11-07T22:39:19Z

...on-ip/src/main/java/org/springframework/integration/ip/dsl/TcpInboundChannelAdapterSpec.java

+ * @since 5.0
+ *
+ */
+public class TcpInboundChannelAdapterSpec<S extends TcpInboundChannelAdapterSpec<S>>


Well, I see that this generic variant works well in the target code.
So, now I'm not sure if we should restrict with target type without generic modification or leave it with that and let to extend in the future easy...
But, eh., typically we don't do that.
And I'm not sure that we are going to extend TcpReceivingChannelAdapter class to have some TcpInboundChannelAdapterSpec extension as well.

Yeah - I based these on the JMS ones - I didn't notice the subclasses there; fixing...

JIRA: https://jira.spring.io/browse/INT-4162 INT-4162: IP DSL - Phase I Connection Factories Phase 2 - Adapters/Gateways Phase 3 - UDP

Remove unnecessary generics.

garyrussell · 2016-11-08T15:02:12Z

@artembilan One issue with this implementation is...

...from(Tcp.inbouncChannelAdapter(Tcp.server(1234).get())...

The connection factory is not a bean, so in the tests I used

...from(Tcp.inboundGateway(server1()))...

so the connection factory is initialized.

Any suggestions for making the first one work - if we inject a Spec instead of the result of the get() perhaps ??

artembilan · 2016-11-08T15:07:09Z

The TcpInboundChannelAdapterSpec can populate provided ConnectionFactory to the ComponentsRegistration and DSL BPP will take care if it need to register a bean or such one is already there.

artembilan

I don't see ComponentsRegistration implementation.
Or you decided to stay away from that (at least for now) or have just missed my comment as I don't see email notification for your latest commit...

artembilan · 2016-11-08T21:06:00Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/AbstractConnectionFactorySpec.java

+	}
+
+	/**
+	 * @param soKeepAlive the socket keepalive option.


Looks like a typo 😄 : keep alive ?

artembilan · 2016-11-08T21:06:42Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/AbstractConnectionFactorySpec.java

+	}
+
+	/**
+	 * @param taskExecutor the task excecutor.


typo: excecutor

artembilan · 2016-11-08T21:07:54Z

...-ip/src/main/java/org/springframework/integration/ip/dsl/TcpClientConnectionFactorySpec.java

+import org.springframework.integration.ip.tcp.connection.TcpNioClientConnectionFactory;
+
+/**
+ * An {@link IntegrationComponentSpec} for {@link AbstractClientConnectionFactory}s.


The JavaDocs doesn't reflect reality of this class.

artembilan · 2016-11-08T21:08:46Z

...-ip/src/main/java/org/springframework/integration/ip/dsl/TcpServerConnectionFactorySpec.java

+import org.springframework.integration.ip.tcp.connection.TcpNioServerConnectionFactory;
+
+/**
+ * An {@link IntegrationComponentSpec} for {@link AbstractServerConnectionFactory}s.


The same JavaDocs problem here.

artembilan · 2016-11-08T21:15:57Z

...egration-ip/src/main/java/org/springframework/integration/ip/dsl/TcpOutboundGatewaySpec.java

+		this.target.setConnectionFactory(connectionFactory);
+	}
+
+	public TcpOutboundGatewaySpec remoteTimeout(long remoteTimeout) {


Would you mind to elaborate why this hasn't been addressed?
Thanks

artembilan · 2016-11-08T21:23:35Z

...on-ip/src/main/java/org/springframework/integration/ip/dsl/UdpInboundChannelAdapterSpec.java

+	}
+
+	/**
+	 * @param soSendBufferSize set the send bufffer size socket option.


Typo: triple buffer for f 😄

artembilan · 2016-11-08T21:25:18Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java

+	 * @return the spec.
+	 * @see UnicastSendingMessageHandler#setSocketExpression(Expression)
+	 */
+	public UdpOutboundChannelAdapterSpec socketExpression(Expression socketExpression) {


socketFunction() with wrapping to the FunctionExpression would be good, too.

artembilan · 2016-11-08T21:28:03Z

...-ip/src/main/java/org/springframework/integration/ip/udp/MulticastSendingMessageHandler.java

@@ -99,6 +102,28 @@ public MulticastSendingMessageHandler(String address, int port,
 		super(address, port, lengthCheck, acknowledge, ackHost, ackPort, ackTimeout);
 	}

+	/**
+	 * Construct UnicastSendingMessageHandler based on the destination SpEL expression to


s/UnicastSendingMessageHandler/MulticastSendingMessageHandler

artembilan · 2016-11-08T21:28:23Z

...-ip/src/main/java/org/springframework/integration/ip/udp/MulticastSendingMessageHandler.java

+	}
+
+	/**
+	 * Construct UnicastSendingMessageHandler based on the destination SpEL expression to


artembilan · 2016-11-08T21:31:10Z

...-integration-ip/src/test/java/org/springframework/integration/ip/dsl/IpIntegrationTests.java

+				.handle(Tcp.outboundGateway(this.client1))
+				.transform(Transformers.objectToString());
+		IntegrationFlowRegistration theFlow = this.flowContext.registration(flow).register();
+		assertThat(theFlow.getMessagingTemplate().convertSendAndReceive("foo", String.class), equalTo("FOO"));


Cool observation for unit testing 😄

artembilan · 2016-11-09T16:58:55Z

:spring-integration-ip:checkstyleMain[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java:21:8: Unused import - org.springframework.expression.Expression. [UnusedImports]

😄

artembilan · 2016-11-09T18:51:28Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java

+	 * @return the spec.
+	 * @see UnicastSendingMessageHandler#setSocketExpression(org.springframework.expression.Expression)
+	 */
+	public UdpOutboundChannelAdapterSpec socketExpression(Function<Message<?>, ?> socketExpressionFunction) {


It's definitely socketFunction().
The fact transformation to a FunctionExpression is out of end-user API.

Plus this must be like Function<Message<?>, DatagramSocket>, just because:

if (this.socketExpression != null) { socket = this.socketExpression.getValue(this.evaluationContext, message, DatagramSocket.class); }

therefore we restrict the target code with particular type and that is much easier to write the final lambda.

artembilan · 2016-11-09T18:53:48Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java

+		super();
+	}
+
+	UdpOutboundChannelAdapterSpec(String destinationExpression) {


Since there is UnicastSendingMessageHandler(Expression destinationExpression), I would like to see UdpOutboundChannelAdapterSpec(Function<Message<?>, Object> destinationFunction)

artembilan · 2016-11-09T18:57:40Z

...on-ip/src/main/java/org/springframework/integration/ip/dsl/TcpInboundChannelAdapterSpec.java

+
+	TcpInboundChannelAdapterSpec(AbstractConnectionFactory connectionFactory) {
+		super(new TcpReceivingChannelAdapter());
+		this.connectionFactory = connectionFactory;


Well, I see your point about ComponentsRegistration.
How about to have alternative ctor for the AbstractConnectionFactorySpec and populate for registration just only result of that?
Any AbstractConnectionFactory variant should be already supplied as a @Bean
WDYT?

I suggests this because I don't know how to answer to your concern in the https://github.com/spring-projects/spring-integration-java-dsl/issues/137 😄

Add ...Spec CTORs to avoid the issue described here: https://github.com/spring-projects/spring-integration-java-dsl/issues/137 Also other PR comments.

artembilan · 2016-11-09T20:19:33Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java

 	 * @return the spec.
 	 * @see UnicastSendingMessageHandler#setSocketExpression(org.springframework.expression.Expression)
 	 */
-	public UdpOutboundChannelAdapterSpec socketExpression(Function<Message<?>, ?> socketExpressionFunction) {
-		this.target.setSocketExpression(new FunctionExpression<>(socketExpressionFunction));
+	public UdpOutboundChannelAdapterSpec socketExpression(Function<Message<?>, DatagramSocket> socketFunction) {


Do you agree with me that socketFunction() sounds better as a name for this kind of method ?

Sorry - I changed the argument, not the name.

See my comment on https://github.com/spring-projects/spring-integration-java-dsl/issues/137

Let me know if you think I should revert that part of the last commit.

I think we can live with your current Spec-aware ctors. They feel much organic if we would like to inline ConnectionFactory instance.
That your comment isn't clear to me yet, but I'm fine to leave it as is.
We spent already enough time on this PR and it is very close for merge. No reason to revert if it doesn't hurt 😄 .
Let's see what the community says!

We spent already enough time on this PR

Thanks for your patience - this was my first complete module; hopefully my next commit will be the last.

artembilan · 2016-11-09T20:26:00Z

...n-ip/src/main/java/org/springframework/integration/ip/dsl/UdpOutboundChannelAdapterSpec.java

+		this.target = new UnicastSendingMessageHandler(destinationExpression);
+	}
+
+	UdpOutboundChannelAdapterSpec(Function<Message<?>, ?> destinationFunction) {


This ctor isn't covered in the Udp.
The same concern for the UdpMulticastOutboundChannelAdapterSpec.
Also I see a generic problem:

Udp.outboundMulticastAdapter("headers['udp_dest']") .lengthCheck(true) .timeToLive(10)

We can't reach an inheritors methods if we call the super.
So, this UdpOutboundChannelAdapterSpec must be with generic specification for itself. Exactly what you had before on the TcpInboundChannelAdapterSpec for example.
Or what we have on the JmsOutboundChannelAdapterSpec or AbstractConnectionFactorySpec.

artembilan · 2016-11-09T21:20:03Z

The Travis failure isn't related to your work, but I wonder if you wouldn't mind taking a fresh look to that JdbcLockRegistryLeaderInitiatorTests.
Thanks

garyrussell · 2016-11-09T21:26:20Z

It's too hard to debug travis problems without test artifacts - added a log adjuster to get info if it occurs on Bamboo.

artembilan · 2016-11-09T21:20:57Z

.../main/java/org/springframework/integration/ip/dsl/AbstractUdpOutboundChannelAdapterSpec.java

@@ -25,24 +25,25 @@
 import org.springframework.messaging.Message;

 /**
- * A {@link MessageHandlerSpec} for {@link UnicastSendingMessageHandler}s.
+ * A {@link MessageHandlerSpec} for UDB {@link org.springframework.messaging.MessageHandler}s.


Typo: UDP ?

artembilan · 2016-11-09T21:21:38Z

.../main/java/org/springframework/integration/ip/dsl/AbstractUdpOutboundChannelAdapterSpec.java

 *
+ * @param <S> the target {@link UdpUnicastOutboundChannelAdapterSpec} implementation type.


s/UdpUnicastOutboundChannelAdapterSpec/AbstractUdpOutboundChannelAdapterSpec

artembilan · 2016-11-09T21:27:34Z

...-integration-ip/src/test/java/org/springframework/integration/ip/dsl/IpIntegrationTests.java

+		Udp.outboundMulticastAdapter("headers['udp_dest']")
+			.lengthCheck(true)
+			.timeToLive(10);
+	}


This test doesn't look finished: no one assertion.
I understand the premise of this, but looks like a visual observation and compiler approval make a staff.
OTOH I understand also that there might be a case when we change the API and just don't have the code to prove a support for proper method chain.
Maybe it really would be good to finish the test for the valid MulticastSendingMessageHandler use-case?..

It's just a compile-time test - before the commit it wouldn't compile. I didn't think it was necessary to run a runtime test; feel free to delete it on merge it you want.

OK. I'll just add in the end an assertion that we have proper instanceOf after Spec evaluation.

artembilan · 2016-11-09T22:05:15Z

According the latest Travis report we have some problem in the MongoDbInboundChannelAdapterIntegrationTests.
I think there is a race condition with all those fixed-rate in config.
And our testWithOnSuccessDisposition() just received two message for the same document. Just because the poller is able to start a new polling task because the previous hasn't finished during expected fixed-rate="200".

Right, that isn't task of your PR, but does it matter when and how do we fix it ?

Your thoughts ?
Thanks

artembilan · 2016-11-09T22:57:47Z

Merged as 65ec234

artembilan requested changes Nov 7, 2016

View reviewed changes

garyrussell added 2 commits November 7, 2016 18:15

INT-4162: TCP/UDP DSL

34e2318

JIRA: https://jira.spring.io/browse/INT-4162 INT-4162: IP DSL - Phase I Connection Factories Phase 2 - Adapters/Gateways Phase 3 - UDP

Polishing - PR Comments

2e095a1

Remove unnecessary generics.

garyrussell force-pushed the INT-4162 branch from 6575243 to 2e095a1 Compare November 8, 2016 14:31

artembilan requested changes Nov 8, 2016

View reviewed changes

More Polishing - PR Comments

8f658cd

Checkstyle/Javadoc Fixes

e546eab

artembilan requested changes Nov 9, 2016

View reviewed changes

ComponentsRegistration Improvements

b018c0b

Add ...Spec CTORs to avoid the issue described here: https://github.com/spring-projects/spring-integration-java-dsl/issues/137 Also other PR comments.

artembilan reviewed Nov 9, 2016

View reviewed changes

artembilan requested changes Nov 9, 2016

View reviewed changes

Fix UDP Spec Inheritance

3a05789

Jdbc Lock Test Log Adjuster

f4e876f

artembilan requested changes Nov 9, 2016

View reviewed changes

Fix Mongo Test Race Conditions

106d585

artembilan closed this Nov 9, 2016

garyrussell deleted the INT-4162 branch March 17, 2017 18:14

spring-operator mentioned this pull request Dec 1, 2016

Add TCP/UDP DSL Specs [INT-4162] #8105

Closed


		protected C target;

		public AbstractConnectionFactorySpec(C connectionFactory) {

		*
		* @param <S> the target {@link UdpUnicastOutboundChannelAdapterSpec} implementation type.

INT-4162: TCP/UDP DSL #1966

INT-4162: TCP/UDP DSL #1966

Uh oh!

Conversation

garyrussell commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garyrussell commented Nov 8, 2016

Uh oh!

artembilan commented Nov 8, 2016

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan commented Nov 9, 2016

Uh oh!

garyrussell commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment